Skip to content

Add support for handling user prompt #15291

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 6 commits into from
Mar 3, 2025
Merged

Add support for handling user prompt #15291

merged 6 commits into from
Mar 3, 2025

Conversation

@aguspe
Copy link
Contributor

@aguspe aguspe commented Feb 16, 2025 •
edited
Loading

User description

Motivation and Context

To add full support of the BiDi protocol of the ruby bindings this PR adds support for handling user prompts through BiDi

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Checklist

  • I have read the contributing document.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I have added tests to cover my changes.
  • All new and existing tests passed.

PR Type

Enhancement, Tests

Description

  • Added handle_user_prompt method to manage user prompts.

  • Introduced tests for handling user prompts in various scenarios.

  • Updated type signature for handle_user_prompt in RBS file.

Changes walkthrough 📝

Relevant files
Enhancement
browsing_context.rb
Add `handle_user_prompt` method for user prompts                 

rb/lib/selenium/webdriver/bidi/browsing_context.rb

  • Added handle_user_prompt method to handle user prompts.
  • Method supports accepting, rejecting, and providing text for prompts.
    		• +4/-0     
    browsing_context.rbs
    Update RBS with `handle_user_prompt` signature                     

    rb/sig/lib/selenium/webdriver/bidi/browsing_context.rbs

  • Added type signature for handle_user_prompt method.
  • Defined parameters and return type for the method.
    		• +2/-0     
    Tests
    browsing_context_spec.rb
    Add tests for handling user prompts                                           

    rb/spec/integration/selenium/webdriver/bidi/browsing_context_spec.rb

  • Added tests for accepting prompts without text.
  • Added tests for accepting prompts with text.
  • Added tests for rejecting prompts.
    		• +34/-2   
    Need help?
  • Type /help how to ... in the comments thread for any questions about Qodo Merge usage.
  • Check out the documentation for more information.
    •

    @qodo-merge-pro
    Copy link
    Contributor

    qodo-merge-pro bot commented Feb 16, 2025

    PR Reviewer Guide 🔍

    Here are some key observations to aid the review process:

    ⏱️ Estimated effort to review: 2 🔵🔵⚪⚪⚪
    🧪 PR contains tests
    🔒 No security concerns identified
    ⚡ Recommended focus areas for review

    Missing Test Setup

    The test cases for handling user prompts don't show how the prompts are triggered. The assertions check for 'hello' and 'goodbye' in page source but there's no setup code showing how these values get there.

    it 'accepts users prompts without text' do
  reset_driver!(web_socket_url: true) do |driver|
    browsing_context = described_class.new(driver)
    window = browsing_context.create

    browsing_context.handle_user_prompt(window, accept: true)

    expect(driver.page_source).to include('hello')
  end
end

it 'accepts users prompts with text' do
  reset_driver!(web_socket_url: true) do |driver|
    browsing_context = described_class.new(driver)
    window = browsing_context.create

    browsing_context.handle_user_prompt(window, accept: true, text: 'Hello, world!')

    expect(driver.page_source).to include('hello')
  end
end

it 'rejects users prompts' do
  reset_driver!(web_socket_url: true) do |driver|
    browsing_context = described_class.new(driver)
    window = browsing_context.create

    browsing_context.handle_user_prompt(window, accept: false)

    expect(driver.page_source).to include('goodbye')
  end
end
    Incorrect Type Signature

    The type signature for handle_user_prompt shows text parameter as required String, but in the implementation it's an optional parameter that can be nil.

    def handle_user_prompt: (String context, bool accept, String text) -> untyped

    @qodo-merge-pro
    Copy link
    Contributor

    qodo-merge-pro bot commented Feb 16, 2025 •
    edited
    Loading

    PR Code Suggestions ✨

    Explore these optional code suggestions:

    CategorySuggestion                                                                                                                                    Impact
    Possible issue
    Fix incorrect method signature

    Update method signature to make text parameter optional and match the
    implementation in the Ruby file.

    rb/sig/lib/selenium/webdriver/bidi/browsing_context.rbs [11]

    -def handle_user_prompt: (String context, bool accept, String text) -> untyped
+def handle_user_prompt: (String context_id, ?accept: bool, ?text: String?) -> untyped
    • Apply this suggestion
    Suggestion importance[1-10]: 9

    __

    Why: The current type signature is incorrect and doesn't match the actual implementation, which could cause type checking errors. The suggestion properly reflects the optional parameters and their types.

    		High
    Validate prompt text input parameters

    Add input validation to ensure text parameter is only provided when accept is
    true, as rejected prompts should not include text input.

    rb/lib/selenium/webdriver/bidi/browsing_context.rb [98-100]

     def handle_user_prompt(context_id, accept: true, text: nil)
+  raise ArgumentError, 'Text can only be provided when accepting a prompt' if !accept && !text.nil?
   @bidi.send_cmd('browsingContext.handleUserPrompt', context: context_id, accept: accept, text: text)
 end
    • Apply this suggestion
    Suggestion importance[1-10]: 8

    __

    Why: The suggestion adds important input validation to prevent invalid combinations of parameters, which could lead to runtime errors or unexpected behavior when rejecting prompts with text.

    		Medium
    Learned
    best practice
    Add parameter validation to prevent nil reference errors by checking required parameters at method start

    Add validation for the required context_id parameter at the start of the
    handle_user_prompt method to prevent potential nil reference errors. Raise
    ArgumentError with a descriptive message if nil is provided.

    rb/lib/selenium/webdriver/bidi/browsing_context.rb [98-100]

     def handle_user_prompt(context_id, accept: true, text: nil)
+  raise ArgumentError, "context_id cannot be nil" if context_id.nil?
   @bidi.send_cmd('browsingContext.handleUserPrompt', context: context_id, accept: accept, text: text)
 end
    • Apply this suggestion
    Suggestion importance[1-10]: 6
    		Low

    @navin772 navin772 added the C-rb Ruby Bindings label Feb 17, 2025
    @aguspe aguspe merged commit 014129f into SeleniumHQ:trunk Mar 3, 2025
    23 checks passed
    sandeepsuryaprasad pushed a commit to sandeepsuryaprasad/selenium that referenced this pull request Mar 23, 2025
    @aguspe @sandeepsuryaprasad
    Add support for handling user prompt (SeleniumHQ#15291)
    de5cee5 
    * Add support for handling user prompt

* Remove missing guards
    @dependabot dependabot bot mentioned this pull request Sep 18, 2025
    Merged
    @dependabot dependabot bot mentioned this pull request Oct 1, 2025
    Closed
    @dependabot dependabot bot mentioned this pull request Oct 24, 2025
    Closed
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

    Reviewers

    @p0deje p0deje p0deje approved these changes

    Assignees

    No one assigned

    Labels

    C-rb Ruby Bindings Review effort 2/5

    Projects

    None yet

    Milestone

    No milestone

    Development

    Successfully merging this pull request may close these issues.

    3 participants

    @aguspe @p0deje @navin772